fetchGit: don't resolve HEAD ref when a specific rev is requested#346
fetchGit: don't resolve HEAD ref when a specific rev is requested#346
Conversation
|
Caution Review failedAn error occurred during the review process. Please try again later. 📝 WalkthroughWalkthroughThe PR refines Git fetching logic to prevent unnecessary network hits when a specific revision is requested without an explicit ref. The code now omits ref resolution against the remote for explicit rev paths, storing the ref in input attributes only when non-empty. A test is added to verify that cached revisions can be fetched without network access. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
When fetchGit is called with a rev but no explicit ref, the code unconditionally called getDefaultRef() which contacts the remote to resolve HEAD. This caused an unnecessary network round-trip (~800ms) even when the requested rev was already in the local cache. Skip resolving the default ref when a rev is specified, since the rev can be fetched directly by its hash. Fixes NixOS#10773.
grahamc
force-pushed
the
push-qsrpowwzxmwy
branch
from
617fe32 to
98f31ca
Compare
| /* When a specific rev is requested without an explicit ref, don't | ||
| resolve the default ref (which would contact the remote). The | ||
| rev can be fetched directly by its hash. */ | ||
| auto ref = originalRef ? *originalRef : !origRev ? getDefaultRef(repoInfo, shallow) : std::string{}; |
There was a problem hiding this comment.
I think the std::string{} will break things:
The ref is used in a call to resolveRev below, which basically asks git to resolve the string $REF^{commit} (to "peel the ref ultimately down to the underlying commit"). ^{commit} (REF="") will fail to parse, so I think we need to figure something out there.
2 participants
Motivation
Fixes NixOS#10773.
Note that I used AI as part of making this PR. It looks sensible to me? But I didn't think as hard about this one as I usually do.
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests